Add lifetime-aware support for `Display` impl of `Ident` #143185

xizheyin · 2025-06-29T10:49:30Z

Fixes #143150

r? compiler

compiler/rustc_resolve/src/late/diagnostics.rs

xizheyin

@rustbot ready

xizheyin · 2025-06-30T03:42:41Z

compiler/rustc_span/src/symbol.rs

+            }
+            return fmt::Display::fmt(&lifetime, f);
+        }
+
        if self.is_raw {


When Ident starts with ', the contents are printed recursively.

xizheyin · 2025-06-30T03:43:36Z

compiler/rustc_span/src/symbol.rs

+        if self.name == kw::StaticLifetime || self.name == kw::UnderscoreLifetime {
+            false
+        } else if let Some(lifetime) = self.as_lifetime() {
+            lifetime.is_raw_guess()
+        } else {
+            self.name.can_be_raw() && self.is_reserved()
+        }


There are two keywords that need to be excluded here.

xizheyin · 2025-06-30T03:48:26Z

compiler/rustc_parse/src/errors.rs

@@ -2176,7 +2176,7 @@ pub(crate) struct KeywordLifetime {
 pub(crate) struct InvalidLabel {
    #[primary_span]
    pub span: Span,
-    pub name: Symbol,
+    pub name: String,


The label needs to be converted to String, otherwise the error message will call to_ident_string, which will print 'break as 'r#break in the following example.

fn main() { 'break: loop { //~ ERROR invalid label name `'break` } }

I haven't checked to see if there are any other label-related errors yet though, there may be similar issues, but it's generally a more borderline case.

(Oh interesting. 'break is actually an invalid label but 'r#break is only a valid label in Rust >=2021) (ignore, not relevant)

Ah, you should be able to use Symbol instead of String here (doesn't make much of a difference here tho) (we could actually suggest escaping via r# (if we have Rust >= 2021) here but that's for another PR)

I found the implementation of Symbol as diagnostic arguments.

rust/compiler/rustc_errors/src/diagnostic_impls.rs

Lines 165 to 169 in f191420

impl IntoDiagArg for Symbol {

fn into_diag_arg(self, path: &mut Option<std::path::PathBuf>) -> DiagArgValue {

self.to_ident_string().into_diag_arg(path)

}

}

It will be transformed to Ident, so if I use Symbol here, It prints invalid label name 'r#break. I may have to use String.

I think this can be a footgun so I'd rather we introduce a newtype that does the label printing behavior - or maybe even further going as to force any symbol to be either newtype wrapped with one indicating a label vs one indicating a lifetime vs non-lifetime non-label. The latter is more churn though

compiler/rustc_resolve/src/late/diagnostics.rs

xizheyin · 2025-07-24T13:47:35Z

r? compiler

fee1-dead · 2025-07-25T08:15:30Z

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -3221,7 +3222,7 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
                                if i != 0 {
                                    binders += ", ";
                                }
-                                binders += x.as_str();
+                                let _ = write!(binders, "{x}");


don't do this

fee1-dead · 2025-07-25T08:17:09Z

compiler/rustc_parse/src/errors.rs

@@ -2176,7 +2176,7 @@ pub(crate) struct KeywordLifetime {
 pub(crate) struct InvalidLabel {
    #[primary_span]
    pub span: Span,
-    pub name: Symbol,
+    pub name: String,


I think this can be a footgun so I'd rather we introduce a newtype that does the label printing behavior - or maybe even further going as to force any symbol to be either newtype wrapped with one indicating a label vs one indicating a lifetime vs non-lifetime non-label. The latter is more churn though

fee1-dead · 2025-07-25T08:18:45Z

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -3185,7 +3186,7 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
                    let mut rm_inner_binders: FxIndexSet<Span> = Default::default();
                    let (span, sugg) = if span.is_empty() {
                        let mut binder_idents: FxIndexSet<Ident> = Default::default();
-                        binder_idents.insert(Ident::from_str(name.unwrap_or("'a")));
+                        binder_idents.insert(name.unwrap_or(Ident::from_str("'a")));


why not store a symbol? also do we not intern 'a?

compiler/rustc_span/src/symbol.rs

Signed-off-by: xizheyin <[email protected]>

xizheyin

This method comments tell me Symbol don't keep the rawness flag or edition. As I understand it, Symbol is purely string content, focusing on string de-duplication and efficient comparisons. Ident handles syntax-level information (including raw flags). So I modify Ident instead of Symbol.

rust/compiler/rustc_span/src/symbol.rs

Lines 2636 to 2645 in f191420

    
               /// This method is supposed to be used in error messages, so it's expected to be 
        
               /// identical to printing the original identifier token written in source code 
        
               /// (`token_to_string`, `Ident::to_string`), except that symbols don't keep the rawness flag 
        
               /// or edition, so we have to guess the rawness using the global edition. 
        
               pub fn to_ident_string(self) -> String { 
        
                   // Avoid creating an empty identifier, because that asserts in debug builds. 
        
                   if self == sym::empty { String::new() } else { Ident::with_dummy_span(self).to_string() } 
        
               } 
        
           }

why not store a symbol? also do we not intern 'a?

So, I choose Ident.

I'd rather we introduce a newtype that does the label printing behavior - or maybe even further going as to force any symbol to be either newtype wrapped with one indicating a label vs one indicating a lifetime vs non-lifetime non-label. The latter is more churn though

Wouldn't this change be a good idea to turn on a separate PR to keep the logic clean.

xizheyin · 2025-07-26T14:57:08Z

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -3228,7 +3228,7 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
                                if i != 0 {
                                    binders += ", ";
                                }
-                                binders += x.as_str();
+                                binders += &x.to_string();


I change it to &x.to_string() which call display.

use push_str

xizheyin · 2025-07-26T14:58:11Z

compiler/rustc_span/src/symbol.rs

+        } else if let Some(lifetime) = self.as_lifetime() {
+            lifetime.is_raw_guess()


where do you use this

I use Ident::as_lifetime here.

fee1-dead · 2025-07-27T01:49:15Z

do we not intern 'a?

fee1-dead · 2025-07-27T01:49:33Z

Wouldn't this change be a good idea to turn on a separate PR to keep the logic clean.

the logic isn't clean in this PR

xizheyin · 2025-07-27T15:06:12Z

do we not intern 'a?

we don't pre-intern 'a. But we pre-intern '_ and 'static.

Signed-off-by: xizheyin <[email protected]>

xizheyin

@rustbot ready

xizheyin · 2025-07-27T15:40:07Z

compiler/rustc_span/src/symbol.rs

@@ -812,6 +812,7 @@ symbols! {
        default_field_values,
        default_fn,
        default_lib_allocator,
+        default_lifetime: "'a",


I pre-intern 'a.

xizheyin · 2025-07-27T15:40:57Z

compiler/rustc_span/src/symbol.rs

+/// A newtype around `Symbol` specifically for label printing.
+/// This ensures type safety when dealing with labels and provides
+/// specialized formatting behavior for label contexts.
+#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
+pub struct LabelSymbol(Symbol);


I create a new type for label print.

xizheyin · 2025-07-27T15:43:59Z

compiler/rustc_resolve/src/late/diagnostics.rs

                                }
-                                binders += x.as_str();
+                                binders.push_str(&x.to_string());


I use push_str.

xizheyin · 2025-07-27T15:44:32Z

compiler/rustc_resolve/src/late/diagnostics.rs

                        (span, sugg)
                    };

                    if higher_ranked {
                        let message = Cow::from(format!(
                            "consider making the {} lifetime-generic with a new `{}` lifetime",
                            kind.descr(),
-                            name.unwrap_or("'a"),
+                            name.unwrap_or(Ident::with_dummy_span(sym::default_lifetime)),


use interned 'a

rustbot assigned SparrowLii Jun 29, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 29, 2025

fmease self-assigned this Jun 29, 2025

This comment has been minimized.

Sign in to view

Urgau reviewed Jun 29, 2025

View reviewed changes

compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved

fmease reviewed Jun 29, 2025

View reviewed changes

compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved

fmease unassigned SparrowLii Jun 29, 2025

fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 29, 2025

xizheyin force-pushed the 143150 branch 2 times, most recently from c9addca to c4edd71 Compare June 30, 2025 03:40

xizheyin commented Jun 30, 2025

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 30, 2025

This comment has been minimized.

Sign in to view

xizheyin changed the title ~~Use code snippet to get lifetime literal name instead of lifetime.ident~~ Add lifetime-aware support for Display impl of Ident Jun 30, 2025

xizheyin force-pushed the 143150 branch 2 times, most recently from d6c180d to 9306466 Compare June 30, 2025 09:12

compiler-errors reviewed Jun 30, 2025

View reviewed changes

compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved

xizheyin force-pushed the 143150 branch from 9306466 to ea1d3c2 Compare June 30, 2025 14:15

ada4a reviewed Jul 4, 2025

View reviewed changes

compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved

xizheyin force-pushed the 143150 branch from ea1d3c2 to 2839221 Compare July 5, 2025 09:23

rustbot assigned fee1-dead and unassigned fmease Jul 24, 2025

fee1-dead requested changes Jul 25, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2025

xizheyin force-pushed the 143150 branch from 2839221 to e5e1b11 Compare July 26, 2025 03:43

This comment has been minimized.

Sign in to view

xizheyin force-pushed the 143150 branch from e5e1b11 to 4f029b7 Compare July 26, 2025 13:43

This comment has been minimized.

Sign in to view

Add ui test error-lifetime-name-issue-143150.rs

131581a

Signed-off-by: xizheyin <[email protected]>

xizheyin force-pushed the 143150 branch from 4f029b7 to 105267e Compare July 26, 2025 13:51

xizheyin commented Jul 26, 2025

View reviewed changes

xizheyin force-pushed the 143150 branch from 105267e to 8798b34 Compare July 27, 2025 15:39

Add lifetime-aware support for Display of Ident and add LabelSymbol

5863963

Signed-off-by: xizheyin <[email protected]>

xizheyin force-pushed the 143150 branch from 8798b34 to 5863963 Compare July 27, 2025 15:43

xizheyin commented Jul 27, 2025

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 27, 2025

	impl IntoDiagArg for Symbol {
	fn into_diag_arg(self, path: &mut Option<std::path::PathBuf>) -> DiagArgValue {
	self.to_ident_string().into_diag_arg(path)
	}
	}

	/// This method is supposed to be used in error messages, so it's expected to be
	/// identical to printing the original identifier token written in source code
	/// (`token_to_string`, `Ident::to_string`), except that symbols don't keep the rawness flag
	/// or edition, so we have to guess the rawness using the global edition.
	pub fn to_ident_string(self) -> String {
	// Avoid creating an empty identifier, because that asserts in debug builds.
	if self == sym::empty { String::new() } else { Ident::with_dummy_span(self).to_string() }
	}
	}

		} else if let Some(lifetime) = self.as_lifetime() {
		lifetime.is_raw_guess()

Add lifetime-aware support for Display impl of Ident #143185

Are you sure you want to change the base?

Add lifetime-aware support for Display impl of Ident #143185

Conversation

xizheyin commented Jun 29, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

xizheyin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

xizheyin commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

xizheyin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fee1-dead commented Jul 27, 2025

Uh oh!

fee1-dead commented Jul 27, 2025

Uh oh!

xizheyin commented Jul 27, 2025

Uh oh!

xizheyin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add lifetime-aware support for `Display` impl of `Ident` #143185

Add lifetime-aware support for `Display` impl of `Ident` #143185

xizheyin commented Jun 29, 2025 •

edited by rustbot

Loading

fmease Jun 30, 2025 •

edited

Loading